Introducing periodic topology mechanism for JedisCluster #3596

yangbodong22011 · 2023-10-25T11:12:01Z

EDIT: The main changes in this PR are as follows:

Add topologyRefreshEnabled and topologyRefreshPeriod to control the periodic topology refresh mechanism.
topologyRefreshExecutor is a single-threaded executor responsible for running TopologyRefreshTask.
Add checkClusterSlotSequence to check whether the cluster slots returned by the server are consecutive and there are no duplicates.
[Test] In JedisClusterTestBase, CLUSTER RESET SOFT was changed to HARD, because SOFT cannot clean up the Redis Cluster configuration and may cause a crash. Please refer to [CRASH] Cluster CLUSTERMSG_EXT_TYPE_FORGOTTEN_NODE redis#12701
[Test] Adjust the cluster-node-timeout in the Makefile to 15s, consistent with the default configuration of Redis.

codecov-commenter · 2023-10-31T02:16:59Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (65ed601) 71.46% compared to head (2ad3381) 71.49%.

❗ Current head 2ad3381 differs from pull request most recent head 9f9f677. Consider uploading reports for the commit 9f9f677 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3596      +/-   ##
============================================
+ Coverage     71.46%   71.49%   +0.02%     
- Complexity     4848     4860      +12     
============================================
  Files           288      288              
  Lines         15464    15511      +47     
  Branches       1095     1105      +10     
============================================
+ Hits          11051    11089      +38     
- Misses         3934     3938       +4     
- Partials        479      484       +5

Files	Coverage Δ
...redis/clients/jedis/DefaultJedisSocketFactory.java	`96.05% <100.00%> (ø)`
...rc/main/java/redis/clients/jedis/JedisCluster.java	`42.04% <100.00%> (+3.67%)`	⬆️
...nts/jedis/providers/ClusterConnectionProvider.java	`81.15% <100.00%> (+1.15%)`	⬆️
...main/java/redis/clients/jedis/ClusterPipeline.java	`75.00% <0.00%> (-12.50%)`	⬇️
...ava/redis/clients/jedis/JedisClusterInfoCache.java	`77.20% <76.74%> (+0.92%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yangbodong22011 · 2023-10-31T02:22:09Z

@sazzad16 The PR is now available for review, and I have updated the top comments with more details, please take a look at it when you are free.

src/main/java/redis/clients/jedis/UnifiedJedis.java

src/main/java/redis/clients/jedis/JedisCluster.java

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

yangbodong22011 · 2023-10-31T09:07:18Z

notice: #3597 has been included in this PR.

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

src/test/java/redis/clients/jedis/JedisClusterTest.java

src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java

solves redis#3595

Co-authored-by: M Sazzadul Hoque <[email protected]>

sazzad16 · 2023-11-02T12:14:10Z

src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java

-        socket.connect(new InetSocketAddress(host.getHostAddress(), hostAndPort.getPort()), connectionTimeout);
+        // Passing 'host' directly will avoid another call to InetAddress.getByName() inside the InetSocketAddress constructor.
+        // For machines with ipv4 and ipv6, but the startNode uses ipv4 to connect, the ipv6 connection may fail.
+        socket.connect(new InetSocketAddress(host, hostAndPort.getPort()), connectionTimeout);


[Adding for future reference:]
Please check #3597 for more information about this change.

yangbodong22011 · 2023-11-02T14:08:20Z

@sazzad16 I will backport it to 4. x and 3. x, WDYT?

sazzad16 · 2023-11-02T14:45:41Z

@yangbodong22011 I think I can manage 4.x. You can do it if you want. I'm not doing it now because there will be a patch release for 4 and I don't want to manage more branches (if I can).

About 3.x, I doubt if there will ever be a new 3 minor release.

yangbodong22011 · 2023-11-02T15:08:12Z

I think I can manage 4.x.

Thank you, that would be great. p.s. I took the initiative to propose a backport, on the one hand because you are busy, and on the other hand, to truly complete it.

About 3.x, I doubt if there will ever be a new 3 minor release.

I believe that this PR is not only an enhancement, but also a fix, and it is worth releasing a 3. x version. If you agree, I can backport to 3.x.

sazzad16 · 2023-11-02T15:36:09Z

You can craft both backport PRs if you want. But I can't promise a new 3 release.

The main changes in this PR are as follows: 1. Add `topologyRefreshEnabled` and `topologyRefreshPeriod` to control the periodic topology refresh mechanism. 2. `topologyRefreshExecutor` is a single-threaded executor responsible for running `TopologyRefreshTask`. 3. Add `checkClusterSlotSequence` to check whether the cluster slots returned by the server are consecutive and there are no duplicates. 4. [Test] In JedisClusterTestBase, `CLUSTER RESET SOFT` was changed to `HARD`, because SOFT cannot clean up the Redis Cluster configuration and may cause a crash. Please refer to redis/redis#12701 5. [Test] Adjust the cluster-node-timeout in the Makefile to 15s, consistent with the default configuration of Redis. --------- * Introducing periodic topology mechanism for JedisCluster * Apply suggestions from sazzad16 Co-authored-by: M Sazzadul Hoque <[email protected]> * Remove topologyRefreshEnabled * Apply suggestions from sazzad16 Co-authored-by: M Sazzadul Hoque <[email protected]> * remove save topologyRefreshPeriod * Move INIT_NO_ERROR_PROPERTY to JedisCluster * add timeout for clusterPeriodTopologyRefreshTest * Update src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java Co-authored-by: M Sazzadul Hoque <[email protected]> --------- Co-authored-by: M Sazzadul Hoque <[email protected]>

yangbodong22011 · 2023-11-03T02:48:39Z

@sazzad16 I've backported it to 4.x #3604 . Since 5.x already exists, 3.x may not be supported by us.

yangbodong22011 requested review from sazzad16 and chayim October 25, 2023 11:52

yangbodong22011 force-pushed the feature-cluster-periodic-topology-refresh branch 5 times, most recently from b50121f to 3100eb8 Compare October 27, 2023 02:00

yangbodong22011 mentioned this pull request Oct 27, 2023

[CRASH] Cluster CLUSTERMSG_EXT_TYPE_FORGOTTEN_NODE redis/redis#12701

Closed

yangbodong22011 force-pushed the feature-cluster-periodic-topology-refresh branch from 3100eb8 to 2a46e65 Compare October 31, 2023 01:50

sazzad16 requested changes Oct 31, 2023

View reviewed changes

src/main/java/redis/clients/jedis/UnifiedJedis.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/JedisCluster.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java Outdated Show resolved Hide resolved

sazzad16 reviewed Oct 31, 2023

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java Outdated Show resolved Hide resolved

sazzad16 reviewed Oct 31, 2023

View reviewed changes

sazzad16 added this to the 5.1.0 milestone Oct 31, 2023

sazzad16 added backport required enhancement labels Oct 31, 2023

sazzad16 requested changes Oct 31, 2023

View reviewed changes

src/test/java/redis/clients/jedis/JedisClusterTest.java Outdated Show resolved Hide resolved

src/test/java/redis/clients/jedis/JedisClusterTest.java Outdated Show resolved Hide resolved

sazzad16 previously approved these changes Oct 31, 2023

View reviewed changes

sazzad16 added the ready to merge label Oct 31, 2023

yangbodong22011 mentioned this pull request Nov 1, 2023

Fix connect socket InetSocketAddress #3597

Closed

sazzad16 reviewed Nov 2, 2023

View reviewed changes

src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java Show resolved Hide resolved

yangbodong22011 dismissed sazzad16’s stale review via f7f2c5a November 2, 2023 12:06

yangbodong22011 and others added 6 commits November 2, 2023 20:10

Introducing periodic topology mechanism for JedisCluster

745a096

solves redis#3595

Apply suggestions from sazzad16

f048a02

Co-authored-by: M Sazzadul Hoque <[email protected]>

Remove topologyRefreshEnabled

6ef65e3

Apply suggestions from sazzad16

439a2a7

Co-authored-by: M Sazzadul Hoque <[email protected]>

remove save topologyRefreshPeriod

a835964

Move INIT_NO_ERROR_PROPERTY to JedisCluster

dee72a2

yangbodong22011 and others added 2 commits November 2, 2023 20:10

add timeout for clusterPeriodTopologyRefreshTest

ac55940

Update src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java

9f9f677

Co-authored-by: M Sazzadul Hoque <[email protected]>

yangbodong22011 force-pushed the feature-cluster-periodic-topology-refresh branch from f7f2c5a to 9f9f677 Compare November 2, 2023 12:10

sazzad16 reviewed Nov 2, 2023

View reviewed changes

sazzad16 added feature and removed enhancement labels Nov 2, 2023

sazzad16 approved these changes Nov 2, 2023

View reviewed changes

sazzad16 merged commit 5a70626 into redis:master Nov 2, 2023
4 checks passed

yangbodong22011 mentioned this pull request Nov 3, 2023

Introducing periodic topology mechanism for JedisCluster (#3596) #3604

Open

sazzad16 removed the ready to merge label Nov 14, 2023

sazzad16 mentioned this pull request Dec 31, 2023

Introducing periodic topology mechanism for JedisCluster #3595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing periodic topology mechanism for JedisCluster #3596

Introducing periodic topology mechanism for JedisCluster #3596

yangbodong22011 commented Oct 25, 2023 •

edited

Loading

codecov-commenter commented Oct 31, 2023 •

edited

Loading

yangbodong22011 commented Oct 31, 2023

yangbodong22011 commented Oct 31, 2023

sazzad16 Nov 2, 2023

yangbodong22011 commented Nov 2, 2023

sazzad16 commented Nov 2, 2023

yangbodong22011 commented Nov 2, 2023

sazzad16 commented Nov 2, 2023

yangbodong22011 commented Nov 3, 2023

Introducing periodic topology mechanism for JedisCluster #3596

Introducing periodic topology mechanism for JedisCluster #3596

Conversation

yangbodong22011 commented Oct 25, 2023 • edited Loading

codecov-commenter commented Oct 31, 2023 • edited Loading

Codecov Report

yangbodong22011 commented Oct 31, 2023

yangbodong22011 commented Oct 31, 2023

sazzad16 Nov 2, 2023

Choose a reason for hiding this comment

yangbodong22011 commented Nov 2, 2023

sazzad16 commented Nov 2, 2023

yangbodong22011 commented Nov 2, 2023

sazzad16 commented Nov 2, 2023

yangbodong22011 commented Nov 3, 2023

yangbodong22011 commented Oct 25, 2023 •

edited

Loading

codecov-commenter commented Oct 31, 2023 •

edited

Loading